-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: Add REST catalog adapter and tests #4241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(withDot)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would it make sense to pull these out into a separate PR so that we get those in sooner? Unless this PR won't take too long to be merged of course
| } | ||
| } | ||
|
|
||
| this.requriedLength = parts.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo in requriedLength
| /** | ||
| * Adaptor class to translate REST requests into {@link Catalog} API calls. | ||
| */ | ||
| public class RESTCatalogAdapter implements RESTClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to have a separate test class for this adapter to make sure routing and other stuff is properly tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. I'm actually thinking that this class will probably be moved to tests. What we want to make available is CatalogHandlers. The adapter is just a simple way to go between the RESTClient API and CatalogHandlers.
|
|
||
| @Override | ||
| public String toString() { | ||
| return "CharSequenceSet({" + Streams.stream(iterator()).collect(Collectors.joining(", ")) + "})"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, Collectors.joining seems to also take prefix, suffix argument, instead of adding them manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
0d1d4e7 to
339cc16
Compare
339cc16 to
ce1b8b9
Compare
This adds an implementation of
RESTClientthat translates REST protocol requests to theCatalogAPI. This makes it possible to test theRESTCatalogimplementation like any other catalog. These tests do not exercise serialization; request and response objects are passed between methods directly.RESTCatalogAdaptoris a simple translation fromRESTClientroutes to catalog handlersCatalogHandlersaccepts request objects, makes the appropriate calls to a catalog, and produces response objectsRESTCatalogis aCatalogimplementation that translates to calls toRESTClientTestRESTCatalogrunsCatalogTestsagainst theRESTCatalogThis also implements load table and update table routes, with new tests in
CatalogTeststhat are passing for REST and JDBC.To get tests working, this PR includes a few other changes and fixes:
equalstoCharSequenceSetfor assertionsTableMetadataParserto handlerefs-1to signal "last added" in schema, partition spec, and sort order changesTableMetadatahistory timestamps by detecting whether a snapshot was addedapplyTo(TableMetadata.Builder)toMetadataUpdateto apply changes from the REST protocol on the server side